Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove mardy_assert from OptionParser to prevent initialisation order issues #358

Closed

Conversation

cniethammer
Copy link
Contributor

MarDyn_assert uses the logger infrastructure of ls1 internally. However the logger is initialized with command line options causing a circular dependence. Therefore, remove mardyn_assert from the OptionParser. If something is wrong with the command line it is early in the execution and therefore no need for the extensive logger infrastructure to be used.

This resolves the segfault brought up during the review of PR #355 (review)

… issues

MarDyn_assert uses the logger infrastructure of ls1 internally. However the
logger is initialized with command line options causing a circular
dependence. Therefore, remove mardyn_assert from the OptionParser.
If something is wrong with the command line it is early in the execution
and therefore no need for the extensive logger infrastructure to be used.

Signed-off-by: Christoph Niethammer <[email protected]>
@FG-TUM
Copy link
Member

FG-TUM commented Dec 11, 2024

The problem you want to solve here is that the logger is only initialized after options are parsed, but the parser might want to log an error. The reason that the logger needs input options is that there can be an input option that specifies a logfile sink.

My suggestion: Initialize the logger without a file sink before parsing the options, and if there is a file sink argument, add the sink to the logger after parsing.
This can either be achieved by slightly refactoring the logger class to offer a set_log_stream(), or just remove the logger and add a new one with a file sink.
This way, we can use the logger and, thus, our exit macro everywhere and have better consistency.

@HomesGH
Copy link
Contributor

HomesGH commented Dec 11, 2024

I agree with @FG-TUM . We could "just" move the init of the Logger from the else clause to some position before the option parsing. Keep in mind that PR #306 is not based on the latest master and a scope was introduced in the meantime.

@cniethammer
Copy link
Contributor Author

I agree, that moving the logger in front would be the way to go. However, for the MPI there is still the problem that the logger currently requires MPI to be initialized inside the constructor as it calls MPI_Comm_rank ... So there will be a bit more refactoring necessary, i.e., removing the MPI dependence from inside the logger...

Note also, that I'd like to have reusable code for other projects, e.g., the option parser was originally a stand-alone thing and could be used in other projects that do not come with ls1 infrastructure (https://github.com/weisslj/cpp-optparse)

@FG-TUM
Copy link
Member

FG-TUM commented Dec 11, 2024

I agree, that moving the logger in front would be the way to go. However, for the MPI there is still the problem that the logger currently requires MPI to be initialized inside the constructor as it calls MPI_Comm_rank ... So there will be a bit more refactoring necessary, i.e., removing the MPI dependence from inside the logger...

Agreed that this dependency can be removed. So we just take the rank id as an argument for the constructor. Yet, I see no need to initialize the logger before MPI_Init(). If that one fails we can't don't log that anyway and just crash with the MPI error. But querying our rank id and initializing the logger with it can immediately be the next thing we do after init.

Note also, that I'd like to have reusable code for other projects, e.g., the option parser was originally a stand-alone thing and could be used in other projects that do not come with ls1 infrastructure (https://github.com/weisslj/cpp-optparse)

If it is copied into the ls1 project, it is fair game. Otherwise, it should be a third party dependency and loaded as such like ADIOS, ALL, AutoPas, ...

@cniethammer cniethammer mentioned this pull request Dec 11, 2024
@cniethammer
Copy link
Contributor Author

Closing in favour of the cleaner solution in #359.

@cniethammer cniethammer deleted the fix-option-parser-segfault branch December 11, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants